-
-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added option to generate basic ape-config.yaml
file if [tool.ape.plugins]
property is specified inside pyproject.toml file [APE-1312]
#1618
feat: added option to generate basic ape-config.yaml
file if [tool.ape.plugins]
property is specified inside pyproject.toml file [APE-1312]
#1618
Conversation
…e.plugins] property is specified inside pyproject.toml file
ape-config.yaml
file if [tool.ape.plugins]
property is specified inside pyproject.toml fileape-config.yaml
file if [tool.ape.plugins]
property is specified inside pyproject.toml file [APE-1312]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some missing things,
and I think there is a helpful standard library to assist with this and simplify the code we have to manage
45b0779
to
6bdce6a
Compare
Co-authored-by: El De-dog-lo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work but things are in the wrong spot and we should be configuring the tool directly instead of generating a config from a config
|
||
def read_dependencies_from_toml(file_path, cli_ctx) -> List[str]: | ||
|
||
with open("./pyproject.toml", "rb") as file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should all happen in the config manage, not in ape-init plugin.
And then you can use the project_manager.path as the base here instead of cwd
return ape_plugins | ||
|
||
|
||
def write_ape_config_yml(dependencies: List[str], file_to_write: Path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of writing an ape file, the ConfigManager should read the values from the pyproject.toml and treat it the same as if the config file had existed. There is not reason to configure twice, even if one is auto-generated.
This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days. |
any update on this one ? |
There is a quite a lot of unaddressed feedback. Basically, we want this to be another source of config rather than creating a config file from it |
so let's make it a draft and wait for further feedback? |
We are not able to provide any more feedback until the approach matches more with the design of having the pyproject.toml be source of config rather than it generating a new config file. |
to expand on this, make it look something like this I suppose: [tools.ape]
plugins = [
etherscan,
vyper,
] |
This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days. |
I think this approach should be closed for now. I am in favor of migrating the |
…
What I did
Added option to specify plugins in the
ape-config.yaml
file based on the[tool.ape.plugins]
property ofpyproject.toml
file.fixes: #1571
How I did it
Added functions for parsing the
pyproject.toml
file & look for lines which have theape-
initials & then stripping them & put them inside theape-config.yaml
file with proper formattingHow to verify it
pyproject.toml
filepoetry install && poetry shell
pip install -e <path/to/the/cloned/repo/with/this/revision/
ape init
POC
Checklist